Resolved offerings #2369

g-linville · 2023-12-05T16:14:15Z

Overview

This change refactors AppInstance.Status.Defaults to AppInstance.Status.ResolvedOfferings. This field used to store the default values for container memory requirements, the region, and volumeclass information for the AppInstance. With these changes, the ResolvedOfferings stores the actual value of the app's region, computeclass information for each container in the app (rather than just memory information), and volumeclass information for each volume. Now it is the source of absolute truth, and there is no need to compare AppInstance.Spec (overrides from the user), AppInstance.Status.AppSpec (defaults from the acorn image), and AppInstance.Status.Defaults (defaults from volume and compute classes) like we used to.

This is a breaking API change. I have updated all of the unit tests in here to reflect that change. (Sorry, there are a lot of them.) I believe this will not cause deployed apps to restart, but they will not get the status updates until they are processed again by the controller (which requires their generation to increase). If we want them to all restart and get the status updates as soon as this runtime change rolls out, I can do that pretty easily as well.

Checklist

The title of this PR would make a good line in Acorn's Release Note's Changelog
The title of this PR ends with a link to the main issue being address in parentheses, like: This is a title (#1216). Here's an example
All relevant issues are referenced in the PR description. NOTE: don't use GitHub keywords that auto-close issues
Commits follow contributing guidance
Automated tests added to cover the changes. If tests couldn't be added, an explanation is provided in the Verification and Testing section
Changes to user-facing functionality, API, CLI, and upgrade impacts are clearly called out in PR description
PR has at least two approvals before merging (or a reasonable exception, like it's just a docs change)

g-linville · 2023-12-05T20:24:28Z

pkg/apis/internal.acorn.io/v1/appinstance.go

+type ResolvedOfferings struct {
+	VolumeSize *resource.Quantity                   `json:"volumeSize,omitempty"`
+	Volumes    map[string]VolumeResolvedOffering    `json:"volumes,omitempty"`
+	Containers map[string]ContainerResolvedOffering `json:"containers,omitempty"`


Instead of storing a map of container name to memory request, we're storing a map of container name to a struct containing the following info:

memory request

computeclass name

cpuscaler value

g-linville · 2023-12-05T20:25:07Z

pkg/controller/appdefinition/deploy.go

+	if offerings := resolvedOfferingsAnnotation(appInstance, container); offerings != "" {
+		annotations[labels.AcornContainerResolvedOfferings] = offerings
+	}


I need to put this annotation onto the ContainerReplica so that I can determine the name of the container's ComputeClass in something else I am working on, where the only info that I have is the ContainerReplica and not the corresponding AppInstance.

g-linville · 2023-12-05T20:40:46Z

pkg/controller/defaults/memory.go

renamed this file to computeclass.go and heavily modified it, so git just thinks I deleted it

g-linville · 2023-12-05T20:40:59Z

pkg/controller/defaults/testdata/volumeclass/volume-class-defaults-set/existing.yaml

this test no longer tests anything useful so I just deleted it

g-linville · 2023-12-05T20:57:18Z

pkg/controller/resolvedofferings/volumeclass_test.go

-func TestVolumeClassDefaultsSet(t *testing.T) {
-	tester.DefaultTest(t, scheme.Scheme, "testdata/volumeclass/volume-class-defaults-set", Calculate)
-}
-


test was no longer useful; deleted

pkg/controller/appdefinition/testdata/volumes/cluster-default-with-bind-combos/expected.golden

Signed-off-by: Grant Linville <[email protected]>

thedadams · 2023-12-08T16:59:27Z

pkg/apis/internal.acorn.io/v1/appinstance.go

-	} else {
-		in.Status.Defaults.Region = ""
-	}
+	in.Status.ResolvedOfferings.Region = region


The intention here was that we only set the region here if it is blank. Some of our code relies on this (e.g. I can call SetDefaultRegion as many times as I want and it will do the right thing).

@thedadams let me know if this function looks good to you now. I am still a little bit confused as to how it is supposed to work to be honest.

thedadams · 2023-12-08T17:29:11Z

pkg/controller/appdefinition/deploy.go

+		data, _ := convert.EncodeToMap(resolved)
+		j, _ := json.Marshal(data)


Not a big fan of ignoring errors here.

👍 yeah, should always handle error in case of bad inputs

pkg/controller/appdefinition/testdata/volumes/cluster-default-with-bind-combos/input.yaml

pkg/controller/scheduling/scheduling.go

thedadams · 2023-12-08T20:13:53Z

pkg/server/registry/apigroups/acorn/apps/validator.go

@@ -258,7 +258,7 @@ func (s *Validator) ValidateUpdate(ctx context.Context, obj, old runtime.Object)
 		}
 	}

-	if newParams.Spec.Region != oldParams.Spec.Region && newParams.Spec.Region != oldParams.Status.Defaults.Region {
+	if newParams.Spec.Region != oldParams.Spec.Region && newParams.Spec.Region != oldParams.Status.ResolvedOfferings.Region {


nit

Suggested change

if newParams.Spec.Region != oldParams.Spec.Region && newParams.Spec.Region != oldParams.Status.ResolvedOfferings.Region {

if newParams.Spec.Region != oldParams.Status.ResolvedOfferings.Region {

Didn't go with the suggestion because it caused update validation issues, but I did modify this line. Let me know what you think.

thedadams · 2023-12-08T20:15:43Z

pkg/volume/volume.go

+	// Order of precedence:
+	// 1. VolumeResolvedOffering - the previously resolved values for Class, Size, and AccessModes
+	// 2. VolumeBinding - the values set by the user in the app spec
+	// 3. VolumeRequest - the defaults as defined in the acorn image


Volume bindings are the ones set by the user at runtime (with the -v flag). I think these are the top precedence.

You're correct. I completely reworked this function.

pkg/apis/api.acorn.io/v1/types.go

StrongMonkey · 2023-12-08T20:24:31Z

pkg/controller/appdefinition/deploy.go

+		data, _ := convert.EncodeToMap(resolved)
+		j, _ := json.Marshal(data)


👍 yeah, should always handle error in case of bad inputs

tylerslaton

Took a look over and the implementation looks correct. I do want to give this a test in my side but approving so you're not blocked on your other work.

tylerslaton · 2023-12-08T20:54:25Z

pkg/controller/appdefinition/volume.go

-					}
-				}
-				pvc.Spec.Resources.Requests[corev1.ResourceStorage] = defaultSize
+				pvc.Spec.Resources.Requests[corev1.ResourceStorage] = *v1.DefaultSize


Why is the above validation no longer required?

Not sure why I removed that, lol. I'll add it back.

Oh wait, nvm. The reason I removed that is because the refactored ResolveVolumeRequest function (called earlier here) will handle setting the default size and checking to see if there is already a size that was previously resolved. So by the time we get down here, if there is still no determined size, the only thing left to use is v1.DefaultSize.

Signed-off-by: Grant Linville <[email protected]>

thedadams · 2023-12-11T12:28:20Z

pkg/apis/internal.acorn.io/v1/appinstance.go

+		if in.Spec.Region != "" && in.Status.ResolvedOfferings.Region == "" {
+			in.Status.ResolvedOfferings.Region = in.Spec.Region
+		} else {
+			in.Status.ResolvedOfferings.Region = region


This logic isn't quite right. For example, if spec.region and status.resolvedOfferings.region are both not empty, then status.resolvedOfferings.region gets set to the passed in region.

I think this is correct:

Suggested change

if in.Spec.Region != "" && in.Status.ResolvedOfferings.Region == "" {

in.Status.ResolvedOfferings.Region = in.Spec.Region

} else {

in.Status.ResolvedOfferings.Region = region

if in.Spec.Region != "" {

in.Status.ResolvedOfferings.Region = in.Spec.Region

} else if in.Status.ResolvedOfferings.Region == "" {

in.Status.ResolvedOfferings.Region = region

I reworked it a little bit. Let me know if you think it looks good. I'm still kinda confused about this.

Honestly, I think what I suggested is simpler. However, what you have does the job.

thedadams · 2023-12-11T13:02:10Z

pkg/controller/resolvedofferings/volumeclass.go

+
+		bound := volumeBindings[name].Volume != ""
+
+		if _, alreadySet := app.Status.ResolvedOfferings.Volumes[name]; !alreadySet && !bound {


This makes it seem like it is not possible to change a volume's settings. For example, if a volume as been resolved and is alreadySet, then would it be possible to change its size or access modes?

The difference of what this did before was that it was only calculating the defaults, and we decided that the defaults for a volume shouldn't change because it causes too many issues. However, now this is the completely "resolved" volume information, which should be allowed to change if the user tries to change it via an Acornfile update (I think your logic allows the user to change volume information via the command line).

Oh, good point. I will work on fixing this.

So, from what I understand, you cannot change the access modes of an existing PVC in Kubernetes, but you can increase its size. I heavily modified this function so that the size can be changed even if the volume offering has been previously resolved, but the volume class and access modes cannot be changed after the volume is created. Let me know if it looks good to you. From my testing, everything seems to me to be working properly.

It doesn't seem to set a different size if a volume is alreadySet and not bound.

thedadams · 2023-12-11T13:21:10Z

pkg/volume/volume.go

@@ -151,24 +151,30 @@ func getVolumeClassNames(volumeClasses map[string]adminv1.ProjectVolumeClassInst
 	return typed.SortedKeys(storageClassName)
 }

-func CopyVolumeDefaults(volumeRequest v1.VolumeRequest, volumeBinding v1.VolumeBinding, volumeDefaults v1.VolumeDefault) v1.VolumeRequest {
+func ResolveVolumeRequest(volumeRequest v1.VolumeRequest, volumeBinding v1.VolumeBinding, resolvedVolume v1.VolumeResolvedOffering) v1.VolumeRequest {


The previous iteration of this function had one responsibility: to put the request, binding and default together to get a merged volume request for the volume to process.

It seems that now this doesn't completely "resolve" the volume. It resolves the request without the defaults and the default information is added later, if needed. In other words, it kind of only does have the job now.

My opinion, which can be completely disregarded, is that passing the default information here and using it to complete "resolve" the volume would be better. Some care would need to be taken so that this function can be uses in the various places it is being used, but I think it would make it more complete.

I reworked the function to fully resolve the volume based on the previously resolved values, all compute class information, bindings specified by the user, etc. Thankfully it didn't require too many modifications in the other places where this function is called. I agree that it is better this way.

Signed-off-by: Grant Linville <[email protected]>

StrongMonkey · 2023-12-13T17:33:44Z

pkg/controller/appdefinition/deploy.go

 func podAnnotations(appInstance *v1.AppInstance, container v1.Container) map[string]string {
 	annotations := map[string]string{
 		labels.AcornContainerSpec: containerAnnotation(container),
 	}
 	addPrometheusAnnotations(annotations, container)

+	offerings, err := resolvedOfferingsAnnotation(appInstance, container)


Looks like this error is still not handled...

Below this line there is a check for if err == nil, and then we attach the annotation. I don't actually want to return this error if it happens, since we can safely just ignore it and keep going.

thedadams · 2023-12-13T17:31:03Z

pkg/apis/internal.acorn.io/v1/appinstance.go

@@ -52,23 +52,23 @@ type AppInstance struct {
 }

 func (in *AppInstance) HasRegion(region string) bool {
-	return in.Status.Defaults.Region == region || in.Spec.Region == region
+	return in.Status.ResolvedOfferings.Region == region || in.Spec.Region == region


Suggested change

return in.Status.ResolvedOfferings.Region == region || in.Spec.Region == region

return in.Status.ResolvedOfferings.Region == region

thedadams · 2023-12-13T17:31:37Z

pkg/apis/internal.acorn.io/v1/appinstance.go

+	if in.Status.ResolvedOfferings.Region != "" {
+		return in.Status.ResolvedOfferings.Region
 	}
-	return in.Status.Defaults.Region
+	return in.Spec.Region


I think we can just return in.Status.ResolvedOfferings.Region here.

thedadams · 2023-12-13T17:45:23Z

pkg/controller/resolvedofferings/volumeclass.go

+
+		bound := volumeBindings[name].Volume != ""
+
+		if _, alreadySet := app.Status.ResolvedOfferings.Volumes[name]; !alreadySet && !bound {


It doesn't seem to set a different size if a volume is alreadySet and not bound.

Signed-off-by: Grant Linville <[email protected]>

This reverts commit dbd950d.

This reverts commit dbd950d. Signed-off-by: Grant Linville <[email protected]>

This reverts commit 8b9acb4.

g-linville commented Dec 5, 2023

View reviewed changes

pkg/controller/appdefinition/testdata/volumes/cluster-default-with-bind-combos/expected.golden Outdated Show resolved Hide resolved

g-linville force-pushed the resolved-offerings branch 2 times, most recently from d06e018 to dcafa98 Compare December 6, 2023 22:15

g-linville changed the title ~~Draft: Resolved offerings~~ Resolved offerings Dec 6, 2023

g-linville marked this pull request as ready for review December 6, 2023 22:44

g-linville requested review from thedadams, njhale, StrongMonkey, iwilltry42, keyallis and tylerslaton December 6, 2023 22:44

g-linville force-pushed the resolved-offerings branch from a011bdb to 3da48c0 Compare December 7, 2023 15:12

Refactor defaults to resolvedOfferings

4aca73a

Signed-off-by: Grant Linville <[email protected]>

g-linville force-pushed the resolved-offerings branch from 3da48c0 to 4aca73a Compare December 7, 2023 16:16

thedadams reviewed Dec 8, 2023

View reviewed changes

StrongMonkey reviewed Dec 8, 2023

View reviewed changes

tylerslaton approved these changes Dec 8, 2023

View reviewed changes

PR changes

5e2b809

Signed-off-by: Grant Linville <[email protected]>

thedadams self-requested a review December 11, 2023 12:44

thedadams reviewed Dec 11, 2023

View reviewed changes

g-linville added 5 commits December 11, 2023 11:10

PR changes (WIP)

9d221f1

Signed-off-by: Grant Linville <[email protected]>

PR changes

abe2384

Signed-off-by: Grant Linville <[email protected]>

Remove accidentally committed file

9f7cc65

Signed-off-by: Grant Linville <[email protected]>

Fix lint issue and remove unneeded function parameter

00b4f52

Signed-off-by: Grant Linville <[email protected]>

Fix issue finding bound volumes

337e1aa

Signed-off-by: Grant Linville <[email protected]>

g-linville requested review from tylerslaton, StrongMonkey and thedadams December 12, 2023 15:21

StrongMonkey approved these changes Dec 13, 2023

View reviewed changes

thedadams reviewed Dec 13, 2023

View reviewed changes

thedadams approved these changes Dec 13, 2023

View reviewed changes

Last changes for suggestions from Donnie

2dfc0ed

Signed-off-by: Grant Linville <[email protected]>

g-linville merged commit dbd950d into acorn-io:main Dec 13, 2023
5 checks passed

g-linville added a commit to g-linville/runtime that referenced this pull request Dec 13, 2023

Revert "Resolved offerings (acorn-io#2369)"

6b15037

This reverts commit dbd950d.

g-linville added a commit to g-linville/runtime that referenced this pull request Dec 13, 2023

Revert "Resolved offerings (acorn-io#2369)"

cab0c3b

This reverts commit dbd950d. Signed-off-by: Grant Linville <[email protected]>

g-linville added a commit that referenced this pull request Dec 13, 2023

Revert "Resolved offerings (#2369)" (#2376)

8b9acb4

This reverts commit dbd950d. Signed-off-by: Grant Linville <[email protected]>

g-linville added a commit to g-linville/runtime that referenced this pull request Dec 14, 2023

Reapply "Resolved offerings (acorn-io#2369)" (acorn-io#2376)

1318768

This reverts commit 8b9acb4.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolved offerings #2369

Resolved offerings #2369

g-linville commented Dec 5, 2023 •

edited

Loading

g-linville Dec 5, 2023

g-linville Dec 5, 2023

g-linville Dec 5, 2023

g-linville Dec 5, 2023

g-linville Dec 5, 2023

thedadams Dec 8, 2023

g-linville Dec 12, 2023

thedadams Dec 8, 2023

StrongMonkey Dec 8, 2023

g-linville Dec 12, 2023

thedadams Dec 8, 2023

g-linville Dec 12, 2023

thedadams Dec 8, 2023

g-linville Dec 12, 2023

StrongMonkey Dec 8, 2023

tylerslaton left a comment

tylerslaton Dec 8, 2023

g-linville Dec 12, 2023

g-linville Dec 12, 2023

thedadams Dec 11, 2023

g-linville Dec 12, 2023

thedadams Dec 13, 2023

thedadams Dec 11, 2023

g-linville Dec 11, 2023

g-linville Dec 12, 2023

thedadams Dec 13, 2023

thedadams Dec 11, 2023

g-linville Dec 12, 2023 •

edited

Loading

StrongMonkey Dec 13, 2023

g-linville Dec 13, 2023

thedadams Dec 13, 2023

thedadams Dec 13, 2023

thedadams Dec 13, 2023

		data, _ := convert.EncodeToMap(resolved)
		j, _ := json.Marshal(data)

	if newParams.Spec.Region != oldParams.Spec.Region && newParams.Spec.Region != oldParams.Status.ResolvedOfferings.Region {
	if newParams.Spec.Region != oldParams.Status.ResolvedOfferings.Region {


		bound := volumeBindings[name].Volume != ""

		if _, alreadySet := app.Status.ResolvedOfferings.Volumes[name]; !alreadySet && !bound {

	return in.Status.ResolvedOfferings.Region == region \|\| in.Spec.Region == region
	return in.Status.ResolvedOfferings.Region == region

Resolved offerings #2369

Resolved offerings #2369

Conversation

g-linville commented Dec 5, 2023 • edited Loading

Overview

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerslaton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-linville Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g-linville commented Dec 5, 2023 •

edited

Loading

g-linville Dec 12, 2023 •

edited

Loading